OLS-3566 Add terminal-run TTL cleanup and oc agentic run cleanup CLI - #413

sequenceDiagram
  participant AgenticRunReconciler
  participant AgenticOLSConfig
  participant KubernetesAPI
  AgenticRunReconciler->>KubernetesAPI: Read terminal AgenticRun
  AgenticRunReconciler->>AgenticOLSConfig: Read lifecycle.terminalTTL
  AgenticRunReconciler->>KubernetesAPI: Persist terminalTime and ttlAfterTerminal
  AgenticRunReconciler->>KubernetesAPI: Requeue until expiration or delete expired run

Mergeability Score: ⚪ Minimal · up to 7b5de

The change adds terminal-run cleanup behavior and configuration-triggered fan-out with only a bounded, non-blocking performance follow-up around full run scans; no actionable merge-blocking risk remains after normal checks and review.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign blublinsky for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Adversarial review

I checked out the branch, built it (go build ./... clean), ran the new tests (all pass), and read the full diff plus surrounding context (reconciler.go, helpers.go, run-lifecycle.md, crd-api.md). Two of the findings below are real bugs that the test suite structurally cannot catch because it uses the fake controller-runtime client, which has none of the eventual-consistency behavior of a real cluster.

🔴 Critical

1. Likely nil-pointer panic in handleTerminalTTL from a cached-client re-Get racing the informer cache

if run.Status.TerminalTime == nil {
    base := run.DeepCopy()
    run.Status.TerminalTime = &now
    if err := r.statusPatch(ctx, run, base); err != nil { ... }
}

if run.Spec.TTLAfterTerminal == nil {
    clusterTTL, err := getTerminalTTL(ctx, r.Client)
    ...
    if clusterTTL != nil {
        // Re-fetch to avoid conflicts after the status patch above.
        if err := r.Get(ctx, client.ObjectKeyFromObject(run), run); err != nil { ... }
        original := run.DeepCopy()
        run.Spec.TTLAfterTerminal = clusterTTL
        ...
    }
}
...
terminalTime := run.Status.TerminalTime.Time  // <-- can panic

r.Client is mgr.GetClient() (see cmd/main.go), the standard delegating client whose reads go through the informer cache while writes go straight to the API server. On the very first reconcile where a run turns terminal and a cluster-wide terminalTTL is already configured:

1. We stamp run.Status.TerminalTime locally and patch the status subresource — the in-memory run now correctly has TerminalTime set.
2. We then call r.Get(...) to "avoid conflicts," which overwrites the entire run struct with whatever the informer cache currently holds. If the watch hasn't observed the write from step 1 yet (a very normal, common race — cache sync is not instantaneous), the fetched copy still has Status.TerminalTime == nil, clobbering the correct value we just set in memory.
3. We fall through to terminalTime := run.Status.TerminalTime.Time → nil pointer dereference.

This is timing-dependent, so it won't always fire, but it's a real production-facing crash risk on the write path that runs on every terminal AgenticRun once cluster TTL is configured. The unit tests (ttl_test.go) use fake.NewClientBuilder(), which is always fully consistent, so they can't reproduce this.

Also worth asking: why re-Get at all? client.MergeFrom (without client.MergeFromWithOptimisticLock) doesn't require a fresh resourceVersion, and controller-runtime's Patch/Status().Patch() already populate the passed object with the server's response. The re-fetch looks unnecessary and is the actual source of the bug — simplest fix is to drop it and patch off the in-memory object directly.

2. TTL stamping bumps metadata.generation, which can spuriously re-arm the revision workflow on terminal runs

run-lifecycle.md rule 6/19 and needsRevision() treat "generation > Analyzed.observedGeneration" as "a new revision was requested." That's only supposed to happen when spec.revisionFeedback changes. But handleTerminalTTL does a plain spec Patch to set TTLAfterTerminal:

original := run.DeepCopy()
run.Spec.TTLAfterTerminal = clusterTTL
if err := r.Patch(ctx, run, client.MergeFrom(original)); err != nil { ... }

Any spec write bumps metadata.generation for a CRD with a status subresource — regardless of which field changed. spec.revisionFeedback is documented as "never cleared" after use (confirmed — there's no code path that resets it), and per rule 6, revision is explicitly supported even from a terminal (NoActionRequired) phase.

Concretely, for an advisory-only run (spec.execution unset) that previously went through one revision cycle and has a stale, already-processed non-empty spec.revisionFeedback sitting in spec: once this run goes terminal again and the TTL reconciler stamps TTLAfterTerminal for the first time, metadata.generation increments past Analyzed.observedGeneration with no actual new feedback. On the next reconcile, needsRevision() returns true again, and:

case agenticv1alpha1.AgenticRunPhaseCompleted, agenticv1alpha1.AgenticRunPhaseFailed:
    if run.Spec.Execution.IsZero() && needsRevision(&run) {
        return r.handleRevision(ctx, &run, resolved)
    }

...re-triggers a full re-analysis (new LLM call, new AnalysisResult, status flips back to Analyzing) for a run that had nothing new to revise. This also means the terminal-phase block that calls handleTerminalTTL gets skipped on that reconcile (since the guard is !(Execution.IsZero() && needsRevision())), so the TTL/cleanup feature partially defeats itself for this run shape. No test in ttl_test.go or cleanup_test.go exercises the TTL-stamp × revisionFeedback interaction, so this regression is invisible to CI.

🟠 Should fix

3. Destructive batch-delete with no confirmation, unlike the CLI's own precedent

oc agentic run cleanup (and -A cross-namespace) permanently deletes AgenticRun history (cascading to owned Result CRs) with only an opt-in --dry-run. Compare to cli/system/suspend.go in this very codebase, which prompts "...Continue? [y/N]" by default and only skips it with an explicit --yes/-y, precisely because it's a consequential cluster-wide action. cleanup is at least as destructive (irreversible audit-trail loss) and has no equivalent safeguard — easy to fat-finger oc agentic run cleanup -A in prod without --dry-run first.

4. Spec docs not updated despite the repo's own rule that they must be

.ai/spec/what/crd-api.md's own "Planned Changes" section says "specs MUST be updated when v1alpha1 changes." This PR adds AgenticOLSConfig.spec.lifecycle.terminalTTL, AgenticRun.spec.ttlAfterTerminal, and AgenticRun.status.terminalTime to the API, and adds a new cleanup subcommand to the CLI, but:

• crd-api.md's "Configuration Surface" and behavioral-rules list don't mention any of the new fields.
• run-lifecycle.md doesn't document the new TTL-driven auto-deletion behavior at all (a fairly significant addition to what "terminal" means operationally).
• cli.md's command tree (oc-agentic run ...) and per-command table don't list cleanup.

Per AGENTS.md/CLAUDE.md for this workspace, specs are supposed to be the source of truth kept in sync with code changes.

5. Unsquashed commits + PR title/commit ticket mismatch

The branch has two commits, both individually titled OLS-3566 ...:

90a8a62 OLS-3566 Add TTL lifecycle CRD fields and reconciler enforcement
bd6bbd0 OLS-3566 Add oc agentic run cleanup CLI command

but the PR title is OLS-3565 Add oc agentic run cleanup CLI command. Per this repo's AGENTS.md: "Squash commits before pushing" and "PR title must start with the Jira reference: OLS-XXXX." Neither is satisfied here — the commits aren't squashed, and the PR title cites a different ticket (OLS-3565) than either commit message (OLS-3566). Worth confirming which ticket is actually correct before merge.

🟡 Minor / nits

• spec.ttlAfterTerminal has no CEL immutability guard, unlike almost every other spec field on AgenticRun (request, targetNamespaces, analysisOutput, tools, analysis, execution, verification are all immutable-after-set via CEL). As written, anyone with patch access can freely rewrite it at any time — including after the operator auto-stamps the cluster-wide default — silently overriding the admin's lifecycle policy for that run. If that's intentional (self-service opt-out), it's worth a one-line doc callout; if not, consider an immutability rule once terminal.
• cli/run/cleanup.go's Run() does an unfiltered client.List of all AgenticRuns (namespace or cluster-wide with -A) and filters client-side. Fine at small scale, but there's no field/label selector, so -A on a large cluster with long-lived history pulls every run into memory every invocation.
• In reconciler.go, the Failed case changed from return r.handleFailed(ctx, &run) to:

  if result, err := r.handleFailed(ctx, &run); err != nil {
      return result, err
  }

  This silently discards result when err == nil. Harmless today only because handleFailed always returns ctrl.Result{} on success — but it's a latent footgun if that function ever grows a requeue path.
• This PR duplicates the entire diff of OLS-3566 Add TTL lifecycle CRD fields and reconciler enforcement #412 (still open/unmerged) via an unsquashed cherry-picked commit rather than stacking on top of it. If OLS-3566 Add TTL lifecycle CRD fields and reconciler enforcement #412 changes during its own review, this branch will silently diverge from what actually lands on main, and reviewers are evaluating the same reconciler/API changes twice in parallel.

What looks solid

• parseDuration's Nd-then-time.ParseDuration fallback, state/older-than filtering, and dry-run table logic in cleanup.go are correct and reasonably tested.
• Terminal-phase detection (isTerminalPhaseIncludingNoAction) correctly matches the reconciler's own isTerminal() + NoActionRequired special case.
• Finalizer interaction: handleTerminalTTL's r.Delete correctly relies on the existing RBAC/templog finalizer cleanup path rather than trying to bypass it.
• Generated CRD YAML matches the Go type changes (make manifests was actually run).

Given findings 1–2 are real correctness bugs that will only surface under real cluster timing/state and aren't covered by any existing or new test, I'd hold this for a fix + regression test before merge.

Update: critical + should-fix items addressed, commits squashed

Pushed a squashed commit (8fecd35) addressing the items from the adversarial review above:

Critical

• Removed the cached-client re-Get in handleTerminalTTL after the terminalTime status patch — it could race the informer cache and clobber the just-stamped value with a stale copy, leading to a nil Status.TerminalTime dereference. Patch already reflects the server response into the in-memory object, so the re-fetch was both unnecessary and the source of the bug.
• Stamping spec.ttlAfterTerminal now advances Analyzed.observedGeneration to the post-patch metadata.generation in the same operation, so this internal spec write can no longer be misread by needsRevision() as a new revision request via stale, already-processed spec.revisionFeedback.
• Added regression tests for both (TestHandleTerminalTTL_StampSyncsObservedGeneration and a note on why the fake client can't naturally reproduce the generation-bump timing — verified manually that the test fails on the pre-fix code and passes after).

Should fix

• cleanup now prints the match table and prompts for confirmation ([y/N]) before deleting, skippable with --yes/-y, mirroring the existing oc agentic suspend pattern. --dry-run still never prompts. Added tests for prompt accept/decline/skip.
• Updated .ai/spec/what/crd-api.md, .ai/spec/what/run-lifecycle.md, and .ai/spec/how/cli.md to document the new fields (spec.lifecycle.terminalTTL, spec.ttlAfterTerminal, status.terminalTime) and the cleanup command, per repo convention.
• Squashed the two commits into one and fixed the PR title to match the commit's ticket (was OLS-3565, commits said OLS-3566 — went with OLS-3566 per the branch name and both original commit messages).

Minor nits (CEL immutability on ttlAfterTerminal, unfiltered cleanup listing at scale, handleFailed result-discard pattern, stacking on unmerged #412) are left as-is; happy to follow up if desired.

go build, go vet, and go test ./... all pass on the squashed branch.

Should-fix: cli.md references non-existent isTerminalPhaseIncludingNoAction

This PR updates IsTerminalPhase in helpers.go to include NoActionRequired, and cleanup.go correctly uses IsTerminalPhase directly. However .ai/spec/how/cli.md was not updated to reflect this:

• Line 52 lists isTerminalPhaseIncludingNoAction in the cleanup.go function table — this function does not exist in the code.
• Line 108 says "IsTerminalPhase does not cover [NoActionRequired] — see isTerminalPhaseIncludingNoAction" — this is now wrong since IsTerminalPhase was updated.
• Line 109 (watch entry) still lists only 4 terminal phases (Completed, Failed, Escalated, Denied), missing NoActionRequired and EmergencyStopped.

Suggest a broader sweep of cli.md to align with the updated IsTerminalPhase.

Should-fix: Duplicate terminal-phase boilerplate in reconciler switch

Same feedback as PR #412 — the sandbox cleanup → audit cleanup → handleTerminalTTL block is copy-pasted across four terminal cases in reconciler.go. A single handleTerminalCleanup(ctx, run, phase) helper would eliminate ~30 lines of duplication.

Nice-to-have: Missing space in crd-api.md configuration surface

spec.verification,spec.ttlAfterTerminal

Should be:

spec.verification`, `spec.ttlAfterTerminal

(Missing comma+space separator between the two fields.)

@sriroopar: all tests passed!

Full PR test history. Your PR dashboard.